Implement CopyToAsync in the FileBufferingReadStream #24499
Conversation
- overrride Span and Memory overloads and implement array overloads in terms of those overloads. - Implemented CopyToAsync (but not CopyTo) - Added tests Fixes #24032
ghost
added
the
| @@ -497,8 +497,8 @@ public async Task ReadAsync_DoesNotDisposeBufferedReadStream() | |||
| var content = "{\"name\": \"Test\"}"; | |||
| var contentBytes = Encoding.UTF8.GetBytes(content); | |||
| var httpContext = GetHttpContext(contentBytes); | |||
| var testBufferedReadStream = new Mock<FileBufferingReadStream>(httpContext.Request.Body, 1024) { CallBase = true }; | |||
| httpContext.Request.Body = testBufferedReadStream.Object; | |||
| var testBufferedReadStream = new VerifyDisposeFileBufferingReadStream(httpContext.Request.Body, 1024); | |||
There was a problem hiding this comment.
Moq doesn't support Span.
There was a problem hiding this comment.
Inline classes would be so nice!
- Added more tests as this was a missing scenario
| { | ||
| ThrowIfDisposed(); | ||
|
|
||
| if (_buffer.Position < _buffer.Length || _completelyBuffered) |
There was a problem hiding this comment.
This logic isn't 100% right, we should be looping in here to fill buffer as much as we can before returning. That isn't a strict contract of Stream though...
There was a problem hiding this comment.
Should I wait for you to do this before reviewing, or do you want to merge this as is for now?
There was a problem hiding this comment.
I'm not making this change in the PR since it isn't a regression.
| async Task CopyToAsyncImpl() | ||
| { | ||
| // At least a 4K buffer | ||
| byte[] buffer = _bytePool.Rent(Math.Min(bufferSize, 4096)); |
There was a problem hiding this comment.
| byte[] buffer = _bytePool.Rent(Math.Min(bufferSize, 4096)); | |
| var buffer = _bytePool.Rent(Math.Min(bufferSize, 4096)); |
| { | ||
| while (true) | ||
| { | ||
| int bytesRead = await ReadAsync(buffer, cancellationToken); |
There was a problem hiding this comment.
| int bytesRead = await ReadAsync(buffer, cancellationToken); | |
| var bytesRead = await ReadAsync(buffer, cancellationToken); |
| @@ -208,39 +208,41 @@ private Stream CreateTempFile() | |||
| FileOptions.Asynchronous | FileOptions.DeleteOnClose | FileOptions.SequentialScan); | |||
| } | |||
|
|
|||
| public override int Read(byte[] buffer, int offset, int count) | |||
| public override int Read(Span<byte> buffer) | |||
There was a problem hiding this comment.
I wonder if we shouldn't do something like the runtime does with its Sync/AsyncReadWriteAdapter to avoid duplicating this somewhat complicated logic. Seems like that way we'd be a lot less likely to introduce bugs in just one version of Read(Async).
I already have your kudos 😄 |
|
Yes, but don't you want extra kudos 😆 |
Try brownies next. |
| @@ -349,6 +361,39 @@ public override void Flush() | |||
| throw new NotSupportedException(); | |||
| } | |||
|
|
|||
| public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) | |||
There was a problem hiding this comment.
How sure are you that you fixed the CopyToAsync issue from #24032? The issue there was that the bufferSize parameter was being set to 1 by the non-virtual CopyToAsync(Stream) if the current stream length was 0 (nothing buffered yet). You're still using the given bufferSize.
amcasey
added
area-networking
Fixes #24032